-
Notifications
You must be signed in to change notification settings - Fork 370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a direct reference to asn1 #8246
Conversation
@ericstj do you know what the guidance will be on transitive packages like this? It's pulled in by MSBuild that has a 6.0.0 dependency. Should I update this to an 8.0.1 version instead of 6.0.1 or just update the minimum amount to avoid the CG alert? |
I think best practice would be CPM + Transitive Pinning. Then you just need to add a package to the list if it shows up in CG. Barring that, I usually reccomend that folks do the following:
In this way you minimize the churn, and place the update next to the dependency you are updating. That said - the whole MSBuild issue is a different sort of problem. There are so many components that reference MSBuild but aren't responsible for deploying it. We really need a better story for plugin / host relationships like this. @baronfel and others are looking at making a way to reference hosts like this without leaking the servicing requirements for the host's dependencies. |
@ericstj sorry if I wasn't clear. I was specifically asking if I should be updating to 6.0.1 since MSBuild depends on 6.0.0 or should I hoist the version all the way up to an 8.0.1 version? Templating wasn't onboarded to CPM until 9. Basically, if we had CPM should I bring in a new major version of the package or just do a smaller version upgrade? |
@dotnet/source-build-internal we're going to need some good guidance around transitive dependencies like these and CG as tooling teams are going to want to add direct references (or CPM upgrades) which will break source build. I thought adding the maestro entry would have worked but it turns out that since I don't have any other runtime dependencies, the source build leg breaks. |
" I thought adding the maestro entry would have worked but it turns out that since I don't have any other runtime dependencies, the source build leg breaks." I think source build will work if you just update the source-build-reference-packages dependency to latest and drop the asn1 dependency all together. The asn1 dependency will get picked up as a reference package. |
There is an open dependency flow PR to update the SBRP dependency. Merge that and remove the System.Formats.Asn1 dependency that was added and I think everything will work. |
I think these dependencies are actually coming from Nuget.Protocols and NuGet.Credentials, not MSBuild. The right answer is "it depends" who's actually shipping the binaries. If it's this project then you can update and I'd say we should update to 8.0 since this is an 8.0 branch and we can keep things automatically up to date with dependency flow if we depend on 8.0. If it's not this project then it's a harder call to make. It depends on what version you can count on the host shipping. For example - if it were VS you'd need to make sure the version you chose matched or was redirected to the one shipped in VS. Similar for SDK or MSBuild. |
This project uses asn1 but I believe the SDK ships it. Looks like because SDK ships it, it ends up loading the 8.0 version so I should probably add that reference rather than 6.0.1. For some reason though, CG isn't flagging the SDK even though it's on 8.0.0 rather than 8.0.1 |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
I think we need to add that to SourceBuild @marcpopMSFT |
The externals package from SB that was recently merged may resolve this. I'm going to rerun and then ping the source build folks for a suggestion (as the other one of these they talked me out of an SBRP change) |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@dotnet/source-build-internal I've updated the version of Asn1 to 8.0.1 which shipped in July with the runtime. Do I need to add this to the reference packages? I had assumed that the source build updates post July would have pulled in the 8.0.1 version of this library. Suggestions? |
@ellahathaway @mthalman @MichaelSimons what is the up to date guidance fir these types of changes? An exclusion in |
An entry for this package should be added to the Version.Details.xml file. This + the version prop in the Versions.props file will cause the dependency to get lifted to the live version while building the VMR (as long as this package is being produced as part of the sb build). Assuming that no prebuilts exist while building the VMR with these changes, this package can be added to the exclusions baseline. I personally don't see a benefit to adding this package to SBRP if it'll get lifted to the live version in the VMR. |
@ellahathaway so I should add the version.details.xml entry even though there is no codeflow from runtime set up. This will enable source build to find the right package and if there is an update in the future, I'll just have to manually update it, correct? |
Since there's no code flow, yes, you'll have to update this dependency manually in the future. Otherwise, SB should now lift the dependency to the live version when building the VMR, but I'm going to test these changes locally to double-check that there's no prebuilts in the VMR. In the meantime, I'll defer to @MichaelSimons or @mthalman since they might have more info. |
@ellahathaway now the build is failing to find the SB intermediate package. Do I need to add that to versions.details.xml as well now? |
I spoke with @mthalman offline, and he indicated that the intermediate shouldn't be necessary in this case. I updated the version.details entry and added system.formats.asn1 to the exclusions baselines... hope that it's ok that I pushed those changes to your branch. I also ported the changes to an 8.0 VMR test branch, and I'm testing the changes in this pipeline run (internal Microsoft link) |
in the most shared project to resolve the CG alerts. This is because pkcs depends on a version of this package that's not compliant but hasn't re-released a new version. That's pulled in by msbuild but the easiest way to fix is just add the direct reference.